Skip to content

rewrite client to use ServiceTemplate based API #154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 13 commits into from

Conversation

niemannd
Copy link
Contributor

⚠️ This PR is a major breaking change! ⚠️

This is a rewrite of the Client and Config classes.

I added a modelMapping to the Config class. This way we are able to specify which index should be mapped to what Java Class.

The Client class now has accessors for the specific apis.
client.index() returns the IndexHandler,
client.documents("indexname") returns the DocumentHandler for the specified index.
client.documents("indexname", Movie.class) returns a DocumentHandler for the specified index with Movie as the desired Java Type, even if there is no mapping in the config for it.
client.instance() returns the InstanceHandler
client.keys() returns the KeysHandler
This way we dont have one uber-class that implements everything.

To make this easier, i created a ClientBuilder class. It allows the user to set every part of the ServiceTemplate classes (HttpClient, JsonHandler, RequestFactory, ServiceTempalte). If nothing gets specified, the ClientBuilder autodetects classes in the Classpath and loads the apropriate Implementation.

@curquiza curquiza added breaking-change The related changes are breaking for the users and removed breaking-change The related changes are breaking for the users labels Mar 28, 2021
@curquiza
Copy link
Member

curquiza commented Apr 6, 2021

Hello @niemannd!
We are busy right now but we will come back to you as soon as possible! 🙂
Thanks for your PR!

@alallema
Copy link
Contributor

alallema commented May 5, 2021

Hi @niemannd !
I’m Amelie. I will take over from Sam, who changed team. First of all, thanks a lot for this PR! Besides the quality of the code than the rewrite of the Readme and Tests and for your clarity, this is nice from you, and I really appreciate it.
However, we want to keep all the SDK and this one too really simple for the users. So the goal is to keep the README and the usage like they are, even if it’s nice to have abstraction.
A default client still needs to be instantiated easily like this:

Client client = new Client(new Config("http://localhost:7700", "masterKey"));

even if we can let the possibility to customize it if we want.
And an index and document should still be accessible like this:

Index index = client.index("books");
index.addDocuments(documents);

Again thanks a lot for what you propose and your work. I understand your purpose not to have one uber class and create more accessibility. Still, this SDK is small and only contains few methods to bring too much complexity for the primary usage it offers.
I think we need to discuss a way to implement a solution that we could easily maintain, especially with simple usage.

@niemannd
Copy link
Contributor Author

niemannd commented Jun 4, 2021

A default client still needs to be instantiated easily like this even if we can let the possibility to customize it if we want

There are a few reasons i chose to not do that.

  1. By circumventing the ClientBuilder you create a second "entrypoint" into the package. Generally i try to avoid that because sooner or later those entrypoints diverge and create additional problems.
    Also i think client = ClientBuilder.withConfig(new Config("http://localhost:7700", "masterKey")).build(); shouldn't be much of a problem?
  2. A second entrypoint is not really user friendly. Software evolves. Telling people they have to change the way they create their client to use some advanced features is ....

And an index and document should still be accessible like this:
Index index = client.index("books");
index.addDocuments(documents);

I chose not to do that because this creates a number of problems.

  1. Coming from an object oriented mindset, in this case you are working on an index-Object, not on the index itself. I would not asume that adding documents to the index-Object triggers any network operation.
  2. This would merge the control layer of the code with the data layer. You have to expect that information about indexes are used in other parts of the application or even across applications. Maybe they are serialized/deserialized it. If at some point the underlying client gets dropped, your index will not be able do perform. Thats why, even for the index, i seperated data class from control class.
  3. This way might work for the add-operation. It wouldn't work for get-operatons. you simply would not know how to deserialize the returned documents.

@alallema
Copy link
Contributor

alallema commented Jun 7, 2021

Hi @niemannd,
I hear what you're saying and I completely understand. It's not an implementation problem but that we have to make choices about what we want to offer to developers and what kind of product we want.
And it's part of the Meili philosophy that the SDK should be as developer-friendly as possible so we don't want to change the README and the usage.
If you still want to talk about it you can join me on slack.

@niemannd
Copy link
Contributor Author

And it's part of the Meili philosophy that the SDK should be as developer-friendly as possible so we don't want to change the README and the usage.

Honestly i don't see the developer-unfriendly part. Do you mean the Index class? A similar pattern is used in the meilisearch-go sdk. Do you mean the ClientBuilder? Again, a similar pattern is used by meilisearch-go and even Spring Data Elasticsearch

So please help me to understand whats the problem with changing the example in the readme of an SDK thats barely used at the moment. If there is a time to change it, its before the sdk is integrated into hundreds of backends.

@alallema
Copy link
Contributor

Hi @niemannd,

A similar pattern is used in the meilisearch-go sdk. Do you mean the ClientBuilder? Again, a similar pattern is used by meilisearch-go and even Spring Data Elasticsearch

We actually do a refactor of the go SDK for the same reason. It is noted that it’s wasn’t clear for the user to pass the name of the index in the function like:
client.documents("books”); -> Seems to say I get the documents “book” not documents inside the index “book”.
For the ClientBuilder it’s really nice to have the possibility to customize it only if we still can have a default client which can be instantiated like this to:
Client client = new Client(new Config("http://localhost:7700", "masterKey”));
I would totally understand if you didn't want to resume your redesign. But if you don't, it would be very cool if you would take back these few points.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants